I definitely like this approach. The only thing I wonder about is if the alg_... variables become type unstable using this approach, but that is probably a negligible effect anyway.

I definitely like this approach. The only thing I wonder about is if the alg_... variables become type unstable using this approach, but that is probably a negligible effect anyway.

I refactored the logic a bit, hopefully it is now friendlier for humans and compilers. (I can double check the type stability but it looks good so far.)

I considered having separate keyword arguments alg_qr and kwargs_qr but that seemed excessive.

I agree that would be excessive and like the current approach. The test breaking seems to be because this is based on an older version where rtol truncation was not yet supported.

I agree that would be excessive and like the current approach. The test breaking seems to be because this is based on an older version where rtol truncation was not yet supported.

Yes, I just merged the changes to #18 into this branch so I'll be able to fix that now. I'll go through and update tests, add new tests, update the other orth and null functions, etc.

Codecov Report

Attention: Patch coverage is 98.46154% with 1 line in your changes missing coverage. Please review.

Ok, this PR is ready for a review, I've updated left_orth, right_orth, left_null, and right_null, along with the docs and tests.

The most awkward part of this new design is deciding how to interpret trunc in left_null and right_null.

The latest design is that if you pass trunc=(; atol, rtol, maxnullity), these decide which small vectors to keep by building a corresponding truncation strategy. maxnullity specifies the maximum size of the null space to output, so is like the complement of maxrank.

However, if you pass a TruncationStrategy to trunc, it is taken literally, so it basically acts the same way orth would act for that TruncationStrategy (i.e. it would keep the same singular vectors as if you passed that trunc to svd_full). I see now that's why the previous design may have been more restrictive about the inputs for the truncation.

The choices I see are:

1. The current design is fine, and it is up to users to pass a good truncation strategy that selects some part of the null space.
2. The truncation strategy could be run on the singular values and then the complement of the ones that are filtered would be used. That seems a bit too tricky/magical.
3. We only allow a NamedTuple to be passed as trunc, but I'm not a huge fan of that since there are reasonable alternative truncation strategies such as picking the null space based on summing the square of singular values and that would be difficult to implement without allowing a generally truncation strategy input.

Curious to hear your thoughts on that, I'm definitely open to suggestions.

Otherwise, given that this is definitely a breaking change, do we want to plan what we want to include in v0.2? It seems like at least the copy_input with alg argument might also be bundled in that release, since that would be breaking. I think the string macros wouldn't be breaking so that could go anywhere inbetween. I would definitely say that we can leave the CUDA stuff to v0.3

That plan sounds good to me. Regarding the string macro, it would be breaking if we decide to delete the aliases and just use string macros to refer to algorithm types. I don't have a very strong opinion on that, just bringing it up as a point of discussion.

It also looks like the branch is out of date with the main, so maybe you could merge/rebase?

I merged, I wasn't able to rebase.

Thanks for all of the comments and corrections, I think I addressed all of them.